address PR review feedback on the Connect Cloud auth refactor - #841
Merged
samperman merged 2 commits intoAug 26, 2026
Merged
Conversation
Reject Connect-only deploy options for Connect Cloud. Reject --draft so the deploy step no longer cites a Connect version for a target that has no draft step at any version. Skip rewriting the store file when a save would not change it. DataStore._set always calls save(), so a Connect Cloud token refresh backed by the system keyring rewrote servers.json on every refresh with contents identical to what was already there, Guard the response status against None before comparing it, since a failed connection produces a response carrying only the exception. Trim the changelog entry, and rewrite two comments review found hard to follow.
|
karawoo
approved these changes
Aug 21, 2026
karawoo
left a comment
Contributor
There was a problem hiding this comment.
One question about the supported deploy options but otherwise lgtm
Comment on lines
+65
to
+72
| _CONNECT_ONLY_DEPLOY_OPTIONS: dict[str, str] = { | ||
| "image": "-I/--image", | ||
| "disable_env_management": "--disable-env-management", | ||
| "env_management_py": "--disable-env-management-py", | ||
| "env_management_r": "--disable-env-management-r", | ||
| "draft": "--draft", | ||
| "metadata": "--metadata", | ||
| } |
Contributor
There was a problem hiding this comment.
Should --disable-env-management-node and --node be included here too? What about --hide-all-input / --hide-tagged-input?
Collaborator
Author
There was a problem hiding this comment.
good catch on the node flags. i've added those to the list. As for the two hide flags, i think i'm going to leave those as is for now and instead investigate how to properly support them on the Connect Cloud side. I've created an issue there to track it.
A user targeting Connect Cloud via a saved nickname can reach deploy_nodejs; reject these options the same way as the other env-management flags.
6 tasks
samperman
added a commit
that referenced
this pull request
Aug 27, 2026
* feat: add support for deploying to Posit Connect Cloud Adds Posit Connect Cloud as a deployment target alongside Posit Connect and shinyapps.io, mirroring the R rsconnect package's support: - Select the target with --connect-cloud or -s connect.posit.cloud. - Authenticate with an interactive OAuth device-code login or a service account client ID/secret (client credentials grant), with automatic token refresh and write-back to the credential store. - Register credentials with `rsconnect add`, verifying the account exists and grants the content:create permission before storing. - Deploy through the Connect Cloud revision model: create or update content, upload the bundle to a presigned URL, publish, poll the revision, and print the publish log from the logs service on failure. - Record deployments locally before publishing, since Connect Cloud cannot look content up by name. - Support the production, staging, and development environments via CONNECT_CLOUD_ENVIRONMENT, pinned to the saved server's URL. Fixes #817 * fix: repair Connect Cloud log authorization on Python 3.8 `cast(dict[str, Any], ...)` evaluates its first argument at runtime, so `from __future__ import annotations` does not cover it and `dict[...]` raises TypeError on Python 3.8. Describe the response with a TypedDict instead, matching the other ConnectCloudClient methods. * fix: make Connect Cloud token refresh failures actionable ConnectCloudClient._attempt_token_refresh logged every failure at debug and returned False, so an expired session or a revoked service account secret surfaced only as the original opaque 401. oauth.py now raises a typed InvalidGrantError when the token endpoint returns error=invalid_grant, carrying the server's error_description. The Cloud client acts on the two credential rejections it can explain: - invalid_grant on the refresh-token path clears connect_cloud_access_token and connect_cloud_refresh_token on the saved servers.json entry (account name/id and nickname are kept) and raises "Your Posit Connect Cloud session has expired and could not be renewed. Authenticate again with `rsconnect add --connect-cloud -n <name> -A <account>`." - invalid_client on the client-credentials path raises a message saying the service account credential was revoked or rotated, pointing at <auth host>/identity/credentials and the `rsconnect add` command with --client-id/--client-secret. The stored entry is left alone. Everything else — network failures, a rejected CLI OAuth client, unexpected responses — still returns False so the original 401 surfaces, but logs at warning instead of debug, matching the Connect refresh path. The servers.json write-back moved into _persist_tokens so the clearing path reuses the field-preserving update. Connect's refresh is unchanged: its generic `except Exception` already covers the new error type. * feat: select Posit Connect Cloud credentials by nickname only A saved Connect Cloud entry is a credential, not an account binding: the login behind it can publish to every account its user has rights on. So -A/--account no longer picks which saved credential to use, only where to publish. With one credential saved it is used whatever account is named; with several, -n/--name is now required and the error lists the saved nicknames with the account each publishes to by default. This drops the account-filter branch and its two error paths from ServerStore._get_connect_cloud_server, along with the now-dead account_name argument to get_by_url and resolve. Behavior change: With several saved credentials, -n selects the credential; -A no longer matches against entries and instead always selects the account to publish to, so `-n cred -A other-account` publishes there with that credential. * feat: keep Posit Connect Cloud credentials in the system keyring Connect Cloud tokens and service account client secrets now go to the system keyring, keyed "<url>#<nickname>" because every Connect Cloud entry records the same API URL. `rsconnect add` and token refresh write there when a keyring is available and leave the matching servers.json fields out, which moves the secrets of an entry saved before this change out of the file on its next add or refresh. Reads prefer the keyring and fall back to those fields, so a machine without a usable keyring (a CI runner) keeps working as before, and `rsconnect server remove` deletes the entries for the removed nickname. `rsconnect list` reports which of the two holds the credentials. The keyring helpers in oauth.py now take the entry key explicitly. Posit Connect keeps passing the bare server URL, so its "<url>:access_token" and "<url>:refresh_token" usernames are unchanged and existing logins are untouched. Tests get a conftest fixture that makes the keyring unavailable by default, since the module is installed in the test environment and would otherwise reach the machine's real keychain. * refactor: share the 401 refresh-and-retry skeleton between both clients RSConnectClient and ConnectCloudClient each had their own copy of "send the request, on 401 mint a new token, send it once more". Both now inherit it from BearerTokenHTTPServer, which calls the subclass's _attempt_token_refresh to mint and apply the token and asks _can_refresh_token whether there is anything to mint from -- false for an API key, a bootstrap JWT, or a Snowflake token exchange. Connect Cloud gains the seekable-body rewind that only the Connect copy had, so a streamed body is not sent empty on the retry. The minting stays per-target, unchanged: Connect keeps discovery against its registered client and the InvalidClientError re-registration recovery, and Connect Cloud keeps the client-credentials-versus-refresh choice and its typed error handling. The keyring-with-servers.json-fallback load and write-back is already the same code on both sides, differing only in the key it is given; what remains target-specific is Connect's token expiry tracking and Connect Cloud's field-preserving write-back, which it skips for a run with no saved entry. Connect's three copies of "find the entry this server came from" become ServerStore.saved_entry. No behavior change other than the added rewind; every existing test passes unmodified. * fix: restore Python 3.8 test collection for test_connect_cloud The stream-body retry tests annotate returns as list[Any], which 3.8 evaluates at class-definition time and rejects. Deferring annotation evaluation with the __future__ import fixes collection for the file. * fix: restore fixture teardown order broken by the keyring conftest The autouse no_system_keyring fixture requested monkeypatch, hoisting the shared per-test instance ahead of every test-level fixture. Its undo then ran after those fixtures' cleanup, so a test using monkeypatch.chdir into a TemporaryDirectory had the directory deleted while it was still the working directory, which Windows rejects (WinError 32 in test_git_metadata teardown). The fixture now saves and restores sys.modules itself. * fix: tell the keyring fixture's None marker from a deleted module entry The teardown guard treated a missing sys.modules key the same as the fixture's own None marker, so a test that deleted the entry would raise KeyError during restore. The sentinel default now separates the cases. * test: cover the keyring fixture teardown branches Extracts the fixture body into an importable generator and adds tests driving each teardown branch: previous module restored, marker removed when nothing was stored, and a deleted key left deleted. The marker is reinstated through a fixture finalizer so a failing assertion cannot leak state into later tests. * test: cover the unknown-nickname error from ServerStore.resolve Nothing asserted this raise; the Connect integration suite exercised it only by accident, and the -n/-A test rework there removed even that. * add support for -V to connect cloud deployments * address PR review feedback on the Connect Cloud auth refactor (#841) * address PR review feedback on the Connect Cloud auth refactor Reject Connect-only deploy options for Connect Cloud. Reject --draft so the deploy step no longer cites a Connect version for a target that has no draft step at any version. Skip rewriting the store file when a save would not change it. DataStore._set always calls save(), so a Connect Cloud token refresh backed by the system keyring rewrote servers.json on every refresh with contents identical to what was already there, Guard the response status against None before comparing it, since a failed connection produces a response carrying only the exception. Trim the changelog entry, and rewrite two comments review found hard to follow. * add --disable-env-management-node and --node to Connect-only options A user targeting Connect Cloud via a saved nickname can reach deploy_nodejs; reject these options the same way as the other env-management flags. * test: take the Connect-only deploy options from validation The test class kept its own copy of the option list, which had already fallen behind: --disable-env-management-node and --node were missing. It now reads validation._CONNECT_ONLY_DEPLOY_OPTIONS, and a new test checks each label against the flags the deploy subcommands actually declare, so a rename in main.py can't leave the rejection message quoting a flag nobody can pass. * test: pin the set of Connect-only deploy options The rejection tests derive their cases from validation._CONNECT_ONLY_DEPLOY_OPTIONS, so deleting an entry there would remove that option's coverage along with it. Pin the key set, so dropping one fails here instead. * refactor: move /v1 into the Connect Cloud logs base URL The api field carried /v1 in the base while logs left it to the one call site, which made the logs entries look like they were missing it. The requests are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reject Connect-only deploy options for Connect Cloud.
Reject --draft so the deploy step no longer cites a Connect version for a target that has no draft step at any version.
Skip rewriting the store file when a save would not change it. DataStore._set always calls save(), so a Connect Cloud token refresh backed by the system keyring rewrote servers.json on every refresh with contents identical to what was already there,
Guard the response status against None before comparing it, since a failed connection produces a response carrying only the exception.
Trim the changelog entry, and rewrite two comments review found hard to follow.
Intent
PR feedback, will be merged into the main PR when ready: #840